feat(imt): add rust implementation of imt #134
Conversation
There was a problem hiding this comment.
I'd suggest minor changes in the current implementation (I showed few directly). For that I'd recommend using Clippy tool. It's basically a linter, that shows how you can fix/change stuff to follow Rust convention of code. You should also use cargo-fmt tool, if you don't do that.
But overall, Is this implementation follows TS implementation fully? I mean not for API but for how is everything implemented inside.
Cause I'd store the tree not as a Vector of Vectors, but for example as a HashMap, where key is a tuple (depth, position) and value is a following leaf/node. Also we don't need to really initialize all tree in the new function, etc. I'd suggest to check how it's done in pmtree .
If you don't want to change the core structure - then everything's good, except minor fixes I mentioned earlier.
Thanks for attention!
There was a problem hiding this comment.
looking at the pmtree implementation you mentioned here, do you think this is still valuable as zk-kit.rust package? @curryrasul
There was a problem hiding this comment.
@0xjei, chatted about that with @curryrasul. We're going to add it to the zk-kit.rust repo as the first package :)
There was a problem hiding this comment.
sweet, tysm for the reply @cedoor
|
Hey @Arch0125, do you think you'll have time to work on the remaining minor changes? We're planning to create a ZK-Kit repo for Rust and it would be nice to have this implementation there :) |
|
Hey @cedoor, yeah ill get done with the minor changes mostly by EOD, or by earlier tomorrow. |
|
@Arch0125 no worries :) Thanks for your quick response! |
There was a problem hiding this comment.
hey @Arch0125 tysm for your work here. I avoided some comments as they were redundant from @curryrasul's review.
In any case, regardless of the design of the solution, I see that the lib.rs is a bit too loaded. It would be nice to have something like:
// this is lib.rs.
pub mod imt;
pub mod hash;by moving the necessary code into imt.rs and hash.rs respectively.
would love to hear your thoughts here as well @curryrasul
|
Updating the code structure in the next commit as mentioned by @0xjei |
|
@Arch0125 This PR should be ready right? Could you solve the conflicts? |
|
Yep solving the conflicts asap |
|
@cedoor ive fixed the remaining issues that came up after the merge |
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2024 ZK-KIT Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
|
@Arch0125 Thank you so much for this PR! As a second step, we need to move this crate to the zk-kit.rust repo. Would you like to do it yourself? |
|
@cedoor Yeah sure, ill take it up, can you assign it there ? |
Sure, here: privacy-scaling-explorations/zk-kit.rust#3. Can you add a comment there? |
Description
This PR adds Rust implmentation of IMT, aligned with the current TS version, with the following methods included insert/update/delete/create-proof/verify-proof
Related Issue
Resolves #133
Does this introduce a breaking change?
Other information
Test coverage
Test New IMT Instance
IMTinstance is created successfully.Test Leaf Insertion
Test Leaf Deletion
Test Leaf Update
Test Create and Verify Proof
Should Not Initialize with Too Many Leaves
arity ^ depth.Should Not Insert in Full Tree
Should Not Delete Nonexistent Leaf